-
Notifications
You must be signed in to change notification settings - Fork 956
Workflow to comment with code review checklists on PRs. #16460
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Add checklist workflow for testing
This reverts commit c788782.
|
Looks like it fails on PRs that are not on the same repository...? It's failing here anyway. |
| on: | ||
| pull_request: | ||
| types: [opened, synchronize, reopened] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah yeah that was a good call to try from a fork. For this type of things (not actually executing foreign code, however running from the context of the base repo default branch) you may need to trigger this on pull_request_target instead:
- https://stackoverflow.com/q/74957218/what-is-the-difference-using-pull-request-target
- https://runs-on.com/github-actions/pull-request-vs-pull-request-target/
(which then has some security considerations mentioned in the links)
That might be necessary for the createComment, updateComment APIs to actually have the token access to write to the PR anyways.
|
Superseded by #16472 |
One-line summary
Workflow to comment with code review checklists on PRs.
Significant changes and points to review
The idea is to create a GitHub workflow which will post appropriate checklists in a comment when a pull request is opened if some file types have been added. So, these are a little general in places. But, I'm hoping they can help keep our code base consistent-ish as we have more new developers contributing.
The conditions where the checklists should post are:
experimentin the nameexperiment_viewvariations = [and the array is not empty=> each condition should only trigger relevant checks
Add checklists for:
Did a bit of markdown cleanup too.
Issue / Bugzilla link
n/a
Testing
You can see an earlier version of the script at work on the pulls here: https://github.com/stephaniehobson/bedrock/pulls
I'll re-run with the most recent version of the script tomorrow.